Skip to content

Fix:S3->Oracle#134

Merged
snu-BuMinSeong merged 5 commits into
developfrom
fix/s3-to-oracle
Apr 15, 2026
Merged

Fix:S3->Oracle#134
snu-BuMinSeong merged 5 commits into
developfrom
fix/s3-to-oracle

Conversation

@snu-BuMinSeong

Copy link
Copy Markdown
Contributor

No description provided.

@snu-BuMinSeong snu-BuMinSeong requested a review from a team as a code owner April 14, 2026 13:46
@coderabbitai

coderabbitai Bot commented Apr 14, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@snu-BuMinSeong has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 28 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 28 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 31e3d3d9-5c5e-4b12-8f3d-c6d63a9595c1

📥 Commits

Reviewing files that changed from the base of the PR and between 06d0d85 and d01d8e5.

📒 Files selected for processing (2)
  • wacruit/src/tests/portfolio/file/conftest.py
  • wacruit/src/tests/portfolio/file/test_portfolio_file_service.py
📝 Walkthrough

Walkthrough

이 PR은 AWS S3 특화 구성에서 OCI Secrets Manager를 통한 자격증명 지원이 있는 범용 객체 저장소 구성으로 리팩토링합니다. S3PortfolioConfigStorageConfig로 이름을 변경하고 사용자 정의 엔드포인트, 액세스 키, 주소 지정 스타일 지원을 추가하며 여러 서비스 및 테스트를 업데이트합니다.

Changes

Cohort / File(s) Summary
Configuration Updates
.gitignore
.env.local.codex를 무시 패턴에 추가했습니다.
Storage Configuration Refactoring
wacruit/src/apps/portfolio/file/aws/config.py
S3PortfolioConfigStorageConfig로 이름 변경; 환경 변수 접두사를 S3_PORTFOLIO_에서 OBJECT_STORAGE_로 변경; region, endpoint_url, access_key_id, secret_access_key, addressing_style 필드 추가; OCI Secrets Manager 통합으로 비밀 조회 로직 업데이트; bucket_region 속성 제거.
S3 Client Initialization
wacruit/src/apps/portfolio/file/aws/s3/client.py
S3Client.__init__을 구성 기반 매개변수(지역, 엔드포인트 URL, AWS 자격증명)를 사용하도록 변경; botocore.config.Config를 추가하여 addressing_style 설정.
Service Configuration Migration
wacruit/src/apps/portfolio/file/services.py, wacruit/src/apps/portfolio/file/services_v2.py, wacruit/src/apps/project/services.py
s3_config에서 storage_config로 종속성 업데이트; S3Client() 호출에서 지역 매개변수 제거; 다중 import 문을 그룹화된 import 블록으로 통합.
Test Utilities
test/check_vault_bucket_name.py, test/oci_s3_connection_test.py
OCI Secrets 및 S3 연결 검증을 위한 새로운 실행 가능한 테스트 스크립트 추가; 환경 변수 로드, OCI Secrets 클라이언트 초기화, 연결성 검증 수행.
Test Updates - Portfolio File Service
wacruit/src/tests/portfolio/file/test_portfolio_file_service.py
하드코딩된 S3 엔드포인트와 자격증명 검증에서 storage_config 기반 속성 검사로 전환; storage_config.endpoint_url 사용 여부에 따라 URL 스키마 검증 조정; moto.mock_s3 데코레이터 제거.
Oracle Integration Tests
wacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.py, wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py, wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py
OCI Object Storage와 Oracle-backed 저장소 비밀에 대한 새로운 엔드-투-엔드 및 계약 테스트 추가; 환경 변수 기반 구성 소스 선택 및 싱글톤 인스턴스 재설정 지원.
Project Service Tests
wacruit/src/tests/project/test_project_service.py
ProjectService 이미지 업로드, 다운로드, 삭제 흐름을 검증하는 새로운 테스트 모듈 추가; storage_config 기반 URL 검증 포함.

Sequence Diagram(s)

sequenceDiagram
    actor App as Application
    participant Config as StorageConfig
    participant Secret as OCISecretManager
    participant S3 as S3-Compatible<br/>Object Storage
    
    App->>Config: Initialize StorageConfig()
    Config->>Secret: Retrieve object_storage_*<br/>secrets (if vault-backed)
    Secret-->>Config: bucket_name, endpoint_url,<br/>credentials, region
    Config-->>App: Configured storage parameters
    
    App->>App: Initialize S3Client()
    App->>S3: Create client with<br/>endpoint_url, region,<br/>addressing_style
    S3-->>App: S3 client ready
    
    App->>S3: Generate presigned POST URL
    S3-->>App: URL + form fields
    App->>S3: Upload object
    S3-->>App: 204/200 response
    
    App->>S3: Generate presigned GET URL
    S3-->>App: Download URL
    App->>S3: Download object
    S3-->>App: Object data
    
    App->>S3: Delete object
    S3-->>App: Success
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Fix:S3->Oracle' is related to the changeset but lacks clarity and specificity about what the fix accomplishes. Provide a more descriptive title that explains the purpose of the S3-to-Oracle migration, such as 'Migrate S3 storage configuration to support Oracle Object Storage' or 'Add Oracle Object Storage support alongside S3'.
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to assess whether the description relates to the changeset. Add a detailed pull request description explaining the S3-to-Oracle migration changes, the motivation for the refactoring, and any breaking changes or configuration updates required.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/s3-to-oracle

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (4)
test/check_vault_bucket_name.py (1)

16-20: 사용되지 않는 헬퍼 함수

_required_env 함수가 정의되어 있지만 이 스크립트 내에서 호출되는 곳이 없습니다. 사용할 계획이 없다면 제거하는 것이 좋습니다.

🔧 제거 제안
-def _required_env(name: str) -> str:
-    value = os.getenv(name, "").strip()
-    if not value:
-        raise ValueError(f"Missing required env: {name}")
-    return value
-
-
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/check_vault_bucket_name.py` around lines 16 - 20, The helper function
_required_env is defined but never used in this script; either delete the
_required_env function to remove dead code or replace existing getenv usage with
calls to _required_env (e.g., where env variables are read) so the helper is
actually utilized; locate the _required_env definition and remove it or refactor
the env retrieval sites to call _required_env accordingly.
wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py (2)

46-68: 헬퍼 함수 내 불필요한 context 재생성

_assert_post_contract_assert_get_contract 내부에서 _build_storage_context()를 다시 호출하고 있습니다(Line 47, 72). 테스트 함수에서 이미 생성한 s3_client를 파라미터로 전달하면 중복 생성을 피할 수 있습니다.

🔧 파라미터로 전달하는 방식 제안
-def _assert_post_contract(storage_config: StorageConfig, object_key: str) -> None:
-    _, s3_client = _build_storage_context()
+def _assert_post_contract(
+    storage_config: StorageConfig, s3_client: BaseClient, object_key: str
+) -> None:
     post_url, fields = generate_presigned_post_url(
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py` around
lines 46 - 68, The helper functions _assert_post_contract and
_assert_get_contract recreate the storage context by calling
_build_storage_context() unnecessarily; change their signatures to accept an
s3_client parameter (instead of calling _build_storage_context inside), remove
the internal _build_storage_context() call, and use the passed s3_client when
calling generate_presigned_post_url / generate_presigned_get_url; update all
test callers to pass the existing s3_client returned by the test setup so no
duplicate clients are created.

25-27: 테스트 파일 간 코드 중복

_reset_singletons(), _target_env(), _build_storage_context() 패턴이 test_oracle_secret_integration.py와 중복됩니다. 공통 테스트 유틸리티 모듈로 추출하면 유지보수성이 향상됩니다.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py` around
lines 25 - 27, The three helper routines _reset_singletons, _target_env, and
_build_storage_context are duplicated across tests; extract them into a shared
test utility module (e.g., tests/util/test_helpers or similar), export those
functions, and replace the duplicated definitions in both
test_oracle_presigned_contract.py and test_oracle_secret_integration.py with
imports from that module; ensure you reference and update usages of
SingletonMeta._instances manipulation in _reset_singletons, the
environment-handling logic in _target_env, and the storage construction in
_build_storage_context so existing tests import the helpers and run unchanged.
wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py (1)

56-81: settings.env 변경 후 복원 누락

테스트에서 settings.env를 변경하지만 원래 값으로 복원하지 않습니다. 이후 실행되는 테스트에 영향을 줄 수 있습니다. pytest fixture나 try-finally 패턴을 사용하여 정리해 주세요.

🔧 fixture를 사용한 정리 패턴 제안
`@pytest.fixture`
def restore_settings_env():
    original_env = settings.env
    yield
    settings.env = original_env
    _reset_singletons()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py` around
lines 56 - 81, test_storage_configuration_is_available mutates settings.env and
doesn't restore it, which can leak state into other tests; update the test to
restore settings.env and call _reset_singletons() after changing it (either by
using a pytest fixture like restore_settings_env that saves and yields the
original settings.env or by wrapping the modification in a try/finally block
inside test_storage_configuration_is_available), ensuring the original value is
reassigned to settings.env in the cleanup path so subsequent tests see the
original environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/check_vault_bucket_name.py`:
- Line 42: The default key name used when reading the env var into key_name is
inconsistent with StorageConfig; change the default from "portfolio_bucket_name"
to "object_storage_bucket_name" so tests align with StorageConfig
expectations—update the assignment that sets key_name (the os.getenv call) to
use "object_storage_bucket_name" as the fallback and ensure any related
assertions referencing StorageConfig.object_storage_bucket_name remain
consistent.

In `@wacruit/src/apps/portfolio/file/aws/config.py`:
- Around line 20-32: The __init__ in the AWS config currently assumes
OCISecretManager.get_secret always returns the requested keys and will raise
KeyError if a secret is missing; update the __init__ to handle missing secrets
by wrapping each call to OCISecretManager.get_secret (for bucket_name, region,
endpoint_url, access_key_id, secret_access_key) with a try/except KeyError (or
using a .get-like fallback) and fall back to sensible defaults such as
environment variables or None; keep the is_available() check but ensure
missing-key handling prevents startup crashes when a particular secret is
absent.

In `@wacruit/src/apps/portfolio/file/aws/s3/client.py`:
- Line 7: S3Client가 모듈 레벨의 전역 storage_config를 고정으로 캡처해 환경 전환 시 이전 엔드포인트/자격증명을 계속
사용하게 되는 문제이므로, S3Client 생성 시 또는 호출 시 최신 설정을 주입하도록 변경하세요; 구체적으로 현재의
storage_config 전역 참조를 제거하고 S3Client.__init__에 storage_config 파라미터(또는 설정을 반환하는
콜백)를 추가하거나, S3Client.get_client()/connect() 같은 메서드 내부에서 런타임에 storage_config를 다시
조회하도록 수정하여 동적 환경 전환을 지원하게 만드세요.

In `@wacruit/src/tests/portfolio/file/test_portfolio_file_service.py`:
- Around line 25-29: The expected_url in the test currently builds the custom
endpoint without a trailing slash causing mismatch with boto3's
generate_presigned_post; update the expression that constructs expected_url (the
conditional using storage_config.endpoint_url and storage_config.bucket_name) to
append a trailing '/' after the bucket for the custom endpoint branch so it
matches the AWS default branch and the URL returned by generate_presigned_post.
- Around line 152-156: The tests fail because StorageConfig (env_prefix =
"OBJECT_STORAGE_") expects OBJECT_STORAGE_BUCKET_NAME but .env.test defines
S3_PORTFOLIO_BUCKET_NAME, so storage_config.bucket_name becomes empty; fix by
either updating .env.test to set
OBJECT_STORAGE_BUCKET_NAME=wacruit-portfolio-test or by setting
storage_config.bucket_name="wacruit-portfolio-test" in the test setup (e.g., in
conftest.py where the bucket is created) so the Key in
test_portfolio_file_service.py uses the actual bucket name.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.py`:
- Around line 50-55: The test helper _build_storage_context creates a new
StorageConfig() and separately instantiates S3Client().client which reads the
module-global storage_config in
wacruit/src/apps/portfolio/file/aws/s3/client.py, and it also mutates
settings.env without restoring it; fix this by creating a single StorageConfig
instance and ensuring the S3 client uses that same instance (e.g., set the
client module's storage_config to the created StorageConfig or construct the
S3Client with that config) and after _reset_singletons() and client creation
restore settings.env to its original value so tests do not leak global env;
update references in _build_storage_context, StorageConfig, and S3Client usage
accordingly.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py`:
- Around line 26-28: The singleton reset function _reset_singletons currently
mutates SingletonMeta._instances without synchronization; modify it to acquire
SingletonMeta._lock while removing entries for OCISecretManager and S3Client to
avoid race conditions. Specifically, inside _reset_singletons obtain
SingletonMeta._lock (e.g., with a context or explicit acquire/release) and then
pop those keys from SingletonMeta._instances, preserving thread safety while
resetting singletons.

In `@wacruit/src/tests/project/test_project_service.py`:
- Around line 112-127: The test currently false-positives because it never
creates the S3 object before calling delete_project_image; ensure the object
exists first, e.g. use the generated presigned URL to PUT the file or call the
S3 client put_object (project_service._s3_client.client.put_object) to upload
"PROJECT/{project.id}/image.png" after
generate_presigned_url_for_post_image/register_project_image_info_in_db, then
call project_service.delete_project_image(upload_response.project_image_id) and
finally assert head_object raises ClientError; reference
generate_presigned_url_for_post_image, register_project_image_info_in_db,
delete_project_image and project_service._s3_client.client.head_object to locate
where to add the pre-delete upload.

---

Nitpick comments:
In `@test/check_vault_bucket_name.py`:
- Around line 16-20: The helper function _required_env is defined but never used
in this script; either delete the _required_env function to remove dead code or
replace existing getenv usage with calls to _required_env (e.g., where env
variables are read) so the helper is actually utilized; locate the _required_env
definition and remove it or refactor the env retrieval sites to call
_required_env accordingly.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py`:
- Around line 46-68: The helper functions _assert_post_contract and
_assert_get_contract recreate the storage context by calling
_build_storage_context() unnecessarily; change their signatures to accept an
s3_client parameter (instead of calling _build_storage_context inside), remove
the internal _build_storage_context() call, and use the passed s3_client when
calling generate_presigned_post_url / generate_presigned_get_url; update all
test callers to pass the existing s3_client returned by the test setup so no
duplicate clients are created.
- Around line 25-27: The three helper routines _reset_singletons, _target_env,
and _build_storage_context are duplicated across tests; extract them into a
shared test utility module (e.g., tests/util/test_helpers or similar), export
those functions, and replace the duplicated definitions in both
test_oracle_presigned_contract.py and test_oracle_secret_integration.py with
imports from that module; ensure you reference and update usages of
SingletonMeta._instances manipulation in _reset_singletons, the
environment-handling logic in _target_env, and the storage construction in
_build_storage_context so existing tests import the helpers and run unchanged.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py`:
- Around line 56-81: test_storage_configuration_is_available mutates
settings.env and doesn't restore it, which can leak state into other tests;
update the test to restore settings.env and call _reset_singletons() after
changing it (either by using a pytest fixture like restore_settings_env that
saves and yields the original settings.env or by wrapping the modification in a
try/finally block inside test_storage_configuration_is_available), ensuring the
original value is reassigned to settings.env in the cleanup path so subsequent
tests see the original environment.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c40ec7ac-4a10-4f9f-adf0-dcab6d150920

📥 Commits

Reviewing files that changed from the base of the PR and between 61603f6 and 06d0d85.

📒 Files selected for processing (13)
  • .gitignore
  • test/check_vault_bucket_name.py
  • test/oci_s3_connection_test.py
  • wacruit/src/apps/portfolio/file/aws/config.py
  • wacruit/src/apps/portfolio/file/aws/s3/client.py
  • wacruit/src/apps/portfolio/file/services.py
  • wacruit/src/apps/portfolio/file/services_v2.py
  • wacruit/src/apps/project/services.py
  • wacruit/src/tests/portfolio/file/test_portfolio_file_service.py
  • wacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.py
  • wacruit/src/tests/portfolio/oracle/test_oracle_presigned_contract.py
  • wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py
  • wacruit/src/tests/project/test_project_service.py

"Missing OCI secret id. Set OCI_SECRET_OCID in .env.test "
"or run with ENV=dev/prod so settings.oci_secret_ocid is available."
)
key_name = os.getenv("OCI_BUCKET_KEY_NAME", "portfolio_bucket_name")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

기본 키 이름이 StorageConfig와 불일치할 수 있음

기본값 portfolio_bucket_nameStorageConfig에서 사용하는 object_storage_bucket_name과 다릅니다. 이 스크립트가 새로운 스토리지 설정을 검증하기 위한 것이라면 기본값을 object_storage_bucket_name으로 변경하는 것이 일관성 있습니다.

🔧 수정 제안
-        key_name = os.getenv("OCI_BUCKET_KEY_NAME", "portfolio_bucket_name")
+        key_name = os.getenv("OCI_BUCKET_KEY_NAME", "object_storage_bucket_name")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
key_name = os.getenv("OCI_BUCKET_KEY_NAME", "portfolio_bucket_name")
key_name = os.getenv("OCI_BUCKET_KEY_NAME", "object_storage_bucket_name")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/check_vault_bucket_name.py` at line 42, The default key name used when
reading the env var into key_name is inconsistent with StorageConfig; change the
default from "portfolio_bucket_name" to "object_storage_bucket_name" so tests
align with StorageConfig expectations—update the assignment that sets key_name
(the os.getenv call) to use "object_storage_bucket_name" as the fallback and
ensure any related assertions referencing
StorageConfig.object_storage_bucket_name remain consistent.

Comment on lines 20 to +32
def __init__(self):
super().__init__()
aws_secrets = OCISecretManager()
if aws_secrets.is_available():
self.bucket_name = aws_secrets.get_secret("portfolio_bucket_name")

@property
def bucket_region(self) -> str:
return "ap-northeast-2"


s3_config = S3PortfolioConfig()
secret_manager = OCISecretManager()
if secret_manager.is_available():
self.bucket_name = secret_manager.get_secret("object_storage_bucket_name")
self.region = secret_manager.get_secret("object_storage_region")
self.endpoint_url = secret_manager.get_secret("object_storage_endpoint_url")
self.access_key_id = secret_manager.get_secret(
"object_storage_access_key_id"
)
self.secret_access_key = secret_manager.get_secret(
"object_storage_secret_access_key"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: OCISecretManager.get_secret 메서드의 에러 처리 확인

ast-grep --pattern $'def get_secret(self, $key) -> str:
  $$$
}'

Repository: wafflestudio/wacruit-server

Length of output: 53


🏁 Script executed:

find . -name "secrets.py" -type f | head -20

Repository: wafflestudio/wacruit-server

Length of output: 95


🏁 Script executed:

rg "class OCISecretManager" -A 20

Repository: wafflestudio/wacruit-server

Length of output: 1436


🏁 Script executed:

cat wacruit/src/apps/portfolio/file/aws/config.py | head -40

Repository: wafflestudio/wacruit-server

Length of output: 1246


🏁 Script executed:

rg "def get_secret" wacruit/src/secrets.py -A 15

Repository: wafflestudio/wacruit-server

Length of output: 602


🏁 Script executed:

cat wacruit/src/secrets.py | tail -50

Repository: wafflestudio/wacruit-server

Length of output: 1742


Vault의 키 누락 시 KeyError 발생 위험

OCISecretManager.get_secret() 메서드는 직접 딕셔너리 접근 (secret_data[key])을 통해 데이터를 반환하며, 요청한 키가 없으면 KeyError를 발생시킵니다. Vault가 사용 가능하다고 해도 (is_available() 반환 True) 특정 키가 누락되어 있으면 애플리케이션 시작 시 실패합니다.

get_secret() 호출을 try-except로 감싸거나 환경 변수 기본값으로 fallback하도록 개선하세요.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/apps/portfolio/file/aws/config.py` around lines 20 - 32, The
__init__ in the AWS config currently assumes OCISecretManager.get_secret always
returns the requested keys and will raise KeyError if a secret is missing;
update the __init__ to handle missing secrets by wrapping each call to
OCISecretManager.get_secret (for bucket_name, region, endpoint_url,
access_key_id, secret_access_key) with a try/except KeyError (or using a
.get-like fallback) and fall back to sensible defaults such as environment
variables or None; keep the is_available() check but ensure missing-key handling
prevents startup crashes when a particular secret is absent.

from botocore.client import BaseClient
from botocore.config import Config

from wacruit.src.apps.portfolio.file.aws.config import storage_config

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

초기화 시점 설정 고정으로 잘못된 스토리지 대상에 연결될 위험이 있습니다.

Line 7, Line 14~24 구조는 S3Client가 전역 storage_config를 단일 인스턴스로 캡처합니다. 환경(settings.env/시크릿 소스) 전환 후에도 이전 endpoint/credential을 계속 사용할 수 있어, 라이브 테스트 및 멀티 환경 실행에서 오동작할 수 있습니다.

🔧 제안 수정안 (설정 주입 가능 + 고정 결합 완화)
-from wacruit.src.apps.portfolio.file.aws.config import storage_config
+from wacruit.src.apps.portfolio.file.aws.config import StorageConfig, storage_config

 class S3Client(metaclass=SingletonMeta):
     _client: BaseClient

-    def __init__(self):
+    def __init__(self, config: StorageConfig | None = None):
+        cfg = config or storage_config
         self._client = cast(
             BaseClient,
             boto3.client(
                 "s3",
-                region_name=storage_config.region,
-                endpoint_url=storage_config.endpoint_url,
-                aws_access_key_id=storage_config.access_key_id,
-                aws_secret_access_key=storage_config.secret_access_key,
-                config=Config(s3={"addressing_style": storage_config.addressing_style}),
+                region_name=cfg.region,
+                endpoint_url=cfg.endpoint_url,
+                aws_access_key_id=cfg.access_key_id,
+                aws_secret_access_key=cfg.secret_access_key,
+                config=Config(s3={"addressing_style": cfg.addressing_style}),
             ),
         )

Also applies to: 14-24

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/apps/portfolio/file/aws/s3/client.py` at line 7, S3Client가 모듈 레벨의
전역 storage_config를 고정으로 캡처해 환경 전환 시 이전 엔드포인트/자격증명을 계속 사용하게 되는 문제이므로, S3Client 생성
시 또는 호출 시 최신 설정을 주입하도록 변경하세요; 구체적으로 현재의 storage_config 전역 참조를 제거하고
S3Client.__init__에 storage_config 파라미터(또는 설정을 반환하는 콜백)를 추가하거나,
S3Client.get_client()/connect() 같은 메서드 내부에서 런타임에 storage_config를 다시 조회하도록 수정하여
동적 환경 전환을 지원하게 만드세요.

Comment on lines +25 to 29
expected_url = (
f"{storage_config.endpoint_url}/{storage_config.bucket_name}"
if storage_config.endpoint_url
else f"https://{storage_config.bucket_name}.s3.amazonaws.com/"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: generate_presigned_post 반환 URL 형식 확인

rg -n "generate_presigned_post" --type py -A 10

Repository: wafflestudio/wacruit-server

Length of output: 12927


🏁 Script executed:

cat -n wacruit/src/tests/portfolio/file/test_portfolio_file_service.py | head -50

Repository: wafflestudio/wacruit-server

Length of output: 2276


🏁 Script executed:

rg -n "expected_url" wacruit/src/tests/portfolio/file/test_portfolio_file_service.py -A 5 -B 5

Repository: wafflestudio/wacruit-server

Length of output: 893


🏁 Script executed:

cat -n wacruit/src/apps/portfolio/file/aws/s3/utils.py | sed -n '88,105p'

Repository: wafflestudio/wacruit-server

Length of output: 560


🏁 Script executed:

cat -n wacruit/src/apps/portfolio/file/services_v2.py | sed -n '111,140p'

Repository: wafflestudio/wacruit-server

Length of output: 1285


🏁 Script executed:

rg -n "PresignedUrlResponse" wacruit/src/apps/portfolio/file/ -A 10

Repository: wafflestudio/wacruit-server

Length of output: 11910


🏁 Script executed:

fd -t f "config.py" wacruit/src/apps/portfolio/file/aws/

Repository: wafflestudio/wacruit-server

Length of output: 116


🏁 Script executed:

cat -n wacruit/src/apps/portfolio/file/aws/config.py

Repository: wafflestudio/wacruit-server

Length of output: 1491


🏁 Script executed:

# boto3 generate_presigned_post 반환 형식 확인
rg -n "def generate_presigned_post" wacruit/src/apps/portfolio/file/aws/s3/utils.py -A 30 | head -40

Repository: wafflestudio/wacruit-server

Length of output: 1210


🏁 Script executed:

# 테스트에서 실제로 비교하는 부분 확인
cat -n wacruit/src/tests/portfolio/file/test_portfolio_file_service.py | sed -n '20,42p'

Repository: wafflestudio/wacruit-server

Length of output: 1128


커스텀 엔드포인트 URL에 trailing slash 추가 필요

26번 줄의 커스텀 엔드포인트 경로에 trailing slash가 누락되어 있습니다. boto3의 generate_presigned_post가 반환하는 URL은 항상 마지막에 /를 포함하므로, 28번 줄의 AWS 기본 경로와 일치하도록 수정하세요.

수정 위치
expected_url = (
    f"{storage_config.endpoint_url}/{storage_config.bucket_name}/"  # trailing slash 추가
    if storage_config.endpoint_url
    else f"https://{storage_config.bucket_name}.s3.amazonaws.com/"
)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/portfolio/file/test_portfolio_file_service.py` around lines
25 - 29, The expected_url in the test currently builds the custom endpoint
without a trailing slash causing mismatch with boto3's generate_presigned_post;
update the expression that constructs expected_url (the conditional using
storage_config.endpoint_url and storage_config.bucket_name) to append a trailing
'/' after the bucket for the custom endpoint branch so it matches the AWS
default branch and the URL returned by generate_presigned_post.

Comment thread wacruit/src/tests/portfolio/file/test_portfolio_file_service.py
Comment on lines +50 to +55
def _build_storage_context() -> tuple[StorageConfig, BaseClient]:
settings.env = cast(Literal["dev", "prod", "local", "test"], _target_env())
_reset_singletons()
storage_config = StorageConfig()
s3_client = S3Client().client

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

스토리지 설정 컨텍스트와 S3 클라이언트 설정 소스가 불일치할 수 있습니다.

Line 53에서 만든 StorageConfig()와 Line 54의 S3Client().client는 동일 설정 객체를 보장하지 않습니다(클라이언트는 wacruit/src/apps/portfolio/file/aws/s3/client.py의 전역 storage_config를 참조). 또한 Line 51의 settings.env 변경을 복구하지 않아 다른 테스트에 전역 상태 오염을 일으킬 수 있습니다.

🔧 제안 수정안 (동일 config로 직접 client 생성 + env 복구)
+import boto3
+from botocore.config import Config
@@
 def _build_storage_context() -> tuple[StorageConfig, BaseClient]:
-    settings.env = cast(Literal["dev", "prod", "local", "test"], _target_env())
-    _reset_singletons()
-    storage_config = StorageConfig()
-    s3_client = S3Client().client
+    previous_env = settings.env
+    try:
+        settings.env = cast(Literal["dev", "prod", "local", "test"], _target_env())
+        _reset_singletons()
+        storage_config = StorageConfig()
+        s3_client = cast(
+            BaseClient,
+            boto3.client(
+                "s3",
+                region_name=storage_config.region,
+                endpoint_url=storage_config.endpoint_url,
+                aws_access_key_id=storage_config.access_key_id,
+                aws_secret_access_key=storage_config.secret_access_key,
+                config=Config(s3={"addressing_style": storage_config.addressing_style}),
+            ),
+        )
@@
-    return storage_config, s3_client
+        return storage_config, s3_client
+    finally:
+        settings.env = previous_env
+        _reset_singletons()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_object_storage_crud.py` around
lines 50 - 55, The test helper _build_storage_context creates a new
StorageConfig() and separately instantiates S3Client().client which reads the
module-global storage_config in
wacruit/src/apps/portfolio/file/aws/s3/client.py, and it also mutates
settings.env without restoring it; fix this by creating a single StorageConfig
instance and ensuring the S3 client uses that same instance (e.g., set the
client module's storage_config to the created StorageConfig or construct the
S3Client with that config) and after _reset_singletons() and client creation
restore settings.env to its original value so tests do not leak global env;
update references in _build_storage_context, StorageConfig, and S3Client usage
accordingly.

Comment on lines +26 to +28
def _reset_singletons() -> None:
for singleton_cls in (OCISecretManager, S3Client):
SingletonMeta._instances.pop(singleton_cls, None)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

싱글톤 리셋 시 스레드 안전성 우려

SingletonMeta._instances.pop()SingletonMeta._lock을 획득하지 않고 직접 인스턴스를 제거합니다. 병렬 테스트 실행 시 경쟁 조건이 발생할 수 있습니다.

🔧 lock을 사용한 안전한 리셋 제안
 def _reset_singletons() -> None:
-    for singleton_cls in (OCISecretManager, S3Client):
-        SingletonMeta._instances.pop(singleton_cls, None)
+    with SingletonMeta._lock:
+        for singleton_cls in (OCISecretManager, S3Client):
+            SingletonMeta._instances.pop(singleton_cls, None)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _reset_singletons() -> None:
for singleton_cls in (OCISecretManager, S3Client):
SingletonMeta._instances.pop(singleton_cls, None)
def _reset_singletons() -> None:
with SingletonMeta._lock:
for singleton_cls in (OCISecretManager, S3Client):
SingletonMeta._instances.pop(singleton_cls, None)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/portfolio/oracle/test_oracle_secret_integration.py` around
lines 26 - 28, The singleton reset function _reset_singletons currently mutates
SingletonMeta._instances without synchronization; modify it to acquire
SingletonMeta._lock while removing entries for OCISecretManager and S3Client to
avoid race conditions. Specifically, inside _reset_singletons obtain
SingletonMeta._lock (e.g., with a context or explicit acquire/release) and then
pop those keys from SingletonMeta._instances, preserving thread safety while
resetting singletons.

Comment on lines +112 to +127
upload_response = project_service.generate_presigned_url_for_post_image(
project_id=project.id,
file_name="image.png",
)
project_service.register_project_image_info_in_db(
file_id=upload_response.project_image_id
)

project_service.delete_project_image(upload_response.project_image_id)

with pytest.raises(ClientError):
project_service._s3_client.client.head_object(
Bucket=storage_config.bucket_name,
Key=f"PROJECT/{project.id}/image.png",
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

삭제 테스트가 거짓 양성(false positive)을 허용합니다.

Line 112~127에서는 실제 S3 객체를 먼저 생성하지 않아도 head_object는 실패합니다. 현재 검증은 “삭제 성공”이 아니라 “원래 없던 객체 조회 실패”도 통과시킬 수 있습니다.

🔧 제안 수정안 (삭제 전 존재 보장)
 def test_delete_project_image(
     project: Project,
     project_service: ProjectService,
 ):
@@
     project_service.register_project_image_info_in_db(
         file_id=upload_response.project_image_id
     )
+    object_key = f"PROJECT/{project.id}/image.png"
+    project_service._s3_client.client.put_object(
+        Bucket=storage_config.bucket_name,
+        Key=object_key,
+        Body=b"smoke-test",
+    )
+    project_service._s3_client.client.head_object(
+        Bucket=storage_config.bucket_name,
+        Key=object_key,
+    )

     project_service.delete_project_image(upload_response.project_image_id)

     with pytest.raises(ClientError):
         project_service._s3_client.client.head_object(
             Bucket=storage_config.bucket_name,
-            Key=f"PROJECT/{project.id}/image.png",
+            Key=object_key,
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
upload_response = project_service.generate_presigned_url_for_post_image(
project_id=project.id,
file_name="image.png",
)
project_service.register_project_image_info_in_db(
file_id=upload_response.project_image_id
)
project_service.delete_project_image(upload_response.project_image_id)
with pytest.raises(ClientError):
project_service._s3_client.client.head_object(
Bucket=storage_config.bucket_name,
Key=f"PROJECT/{project.id}/image.png",
)
upload_response = project_service.generate_presigned_url_for_post_image(
project_id=project.id,
file_name="image.png",
)
project_service.register_project_image_info_in_db(
file_id=upload_response.project_image_id
)
object_key = f"PROJECT/{project.id}/image.png"
project_service._s3_client.client.put_object(
Bucket=storage_config.bucket_name,
Key=object_key,
Body=b"smoke-test",
)
project_service._s3_client.client.head_object(
Bucket=storage_config.bucket_name,
Key=object_key,
)
project_service.delete_project_image(upload_response.project_image_id)
with pytest.raises(ClientError):
project_service._s3_client.client.head_object(
Bucket=storage_config.bucket_name,
Key=object_key,
)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@wacruit/src/tests/project/test_project_service.py` around lines 112 - 127,
The test currently false-positives because it never creates the S3 object before
calling delete_project_image; ensure the object exists first, e.g. use the
generated presigned URL to PUT the file or call the S3 client put_object
(project_service._s3_client.client.put_object) to upload
"PROJECT/{project.id}/image.png" after
generate_presigned_url_for_post_image/register_project_image_info_in_db, then
call project_service.delete_project_image(upload_response.project_image_id) and
finally assert head_object raises ClientError; reference
generate_presigned_url_for_post_image, register_project_image_info_in_db,
delete_project_image and project_service._s3_client.client.head_object to locate
where to add the pre-delete upload.

@snu-BuMinSeong snu-BuMinSeong merged commit 5e587fa into develop Apr 15, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant